Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DYN-6631 - Input/Output Node - part 2 (View customization) #15022

Merged
merged 22 commits into from
Apr 18, 2024

Conversation

dnenov
Copy link
Collaborator

@dnenov dnenov commented Mar 15, 2024

Purpose

This is a follow-up on the work done in #14987 for the creation of a new DefineData node in Dynamo. This PR introduces the beginning of the view customization and stops with the ability to 'validate' the input fed into the node, without going further.

Using the 'unlocked' mode, passing data of one of the valid data types would cause the node to automatically assume the 'correct' data type, including single/list value.

DynamoSandbox_EWxmnVmVhY

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

  • data class refactor
  • introduction of the view customization
  • tests refactor

Reviewers

@saintentropy
@twastvedt

FYIs

@mjkkirschner

dnenov and others added 15 commits February 5, 2024 15:36
- starting with the model and the test suite
- now types are contained inside an enum
- added the basic primitives to the test structure, including lists checks
- reworked the node to start getting the customization going (and make sense of the whole thing)
- created hierarchical container capable of tracking type inheritance
- added all geometry tests
- finished all primitive date type tests
This reverts commit 1621855.
- completed tests checking inf inheritance works on individual and on list level
- added a few additional comments
- additional test checking if inheritance stops at the desired level (ie. `Cone` does not inherit from `Curve`)
- removed the Enum, replaced directly with Type
- removed dictionary in favor of list of datatypes
- renamed methods to better suit the specificity of the node functionality they were serving
- now returns the input as an output
- not doing much with the result of the validation though, except returning to the first value of the drop-down
- added functional IsAutoMode and IsList inputs to the Model
- removed the ViewModel
@dnenov dnenov requested a review from twastvedt March 15, 2024 16:02
@dnenov dnenov changed the title Definedata view DYN-6607 - Input/Output Node - part 2 (View customization) Mar 15, 2024
@dnenov dnenov marked this pull request as ready for review March 15, 2024 16:04
Copy link

github-actions bot commented Mar 15, 2024

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

@twastvedt twastvedt changed the title DYN-6607 - Input/Output Node - part 2 (View customization) DYN-6631 - Input/Output Node - part 2 (View customization) Mar 18, 2024
- do not change dropdown if not in AutoMode
- added detailed description to IsAutoMode property
- public static method of retrieving the list of datatypes replaced by an internal static property DataNodeDynamoTypeList
- IsSupportedDataNodeDynamoType now internal
- refactor
[JsonProperty]
public bool IsList { get; set; }
public bool IsAutoMode
Copy link
Member

@mjkkirschner mjkkirschner Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnenov @twastvedt @saintentropy when this node's data types do not match and errors are thrown, does this node return null?

Does it make sense to explore not returning anything and to stop all downstream computation? For example, this node could compile to an imperative AST like scopeIF.

Anyhow, that may be out of scope and we could do it later - it just seems like a large potential waste of compute if this node is used upstream of the entire graph in many cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does (should) return null/nothing. Makes sense to me. Is that (stopping downstream computation) something that other nodes do? If it's not complicated, perhaps we can look at it now. Things left for later tend not to get done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you point me to an existing implementation, I can take that on, up to you @mjkkirschner and @twastvedt !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@twastvedt no - returning null does not stop downstream computation - it passes null and likely leads to a bunch of slow exceptions for all downstream node calls - that's why I am suggesting rethinking that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look here - but it's not simple, if it's something vera team is interested in it probably warrants a meeting as it's a new scope/feature for sure.

[NodeName("ScopeIf"), NodeCategory(BuiltinNodeCategories.LOGIC),

@dnenov dnenov mentioned this pull request Apr 18, 2024
9 tasks
@twastvedt twastvedt merged commit c9f6dda into DynamoDS:master Apr 18, 2024
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants